Add bulk transformations functionality #49
Conversation
Virviil
force-pushed
the
encoding_transformations
branch
from
3c1d169
to
aba8549
Compare
|
The general direction LGTM! |
Virviil
force-pushed
the
encoding_transformations
branch
from
aba8549
to
c1c3077
Compare
| @spec pad(non_neg_integer(), Tokenizers.Encoding.padding_opts()) :: | ||
| {:pad, {non_neg_integer(), Tokenizers.Encoding.padding_opts()}} |
There was a problem hiding this comment.
I moved option types directly to the relevant functions, so we no longer have that type. We can bring it back, but it's also fine to just say keyword() since we point to the user to Encoding.pad/3 anyway :)
| @spec pad(non_neg_integer(), Tokenizers.Encoding.padding_opts()) :: | |
| {:pad, {non_neg_integer(), Tokenizers.Encoding.padding_opts()}} | |
| @spec pad(non_neg_integer(), keyword()) :: {:pad, {non_neg_integer(), keyword()}} |
There was a problem hiding this comment.
@jonatanklosko I'm not sure that having opts inside function spec and not as a type is good idea (at least here). Using keyword() removes the ElixirLS ability to autosuggest, while having full list of opts in every function is a duplication and can lead to inconsistency, when in the future commits one place will be updated and other - no. WDYT?
There was a problem hiding this comment.
Ah, I wasn't aware ElixirLS does that, I don't have strong opinion, but that's a fair argument. So in this case we can bring the type back to share it :)
There was a problem hiding this comment.
Yep, this is how it looks:
It take it from spec, so keyword() is less expressive.
I'll take a look how to return types at least for these functions
Co-authored-by: Jonatan Kłosko <[email protected]>
Bulk encoding transformations added as discussed here: #44 (comment)
Naming and API discussible. Hope I didn't miss something.